Repository navigation
feat(telemetry): add simulation run context - #266
Conversation
fercor-cisco
left a comment
fercor-cisco
left a comment
There was a problem hiding this comment.
A run ID that groups spans across conversations is useful, and you've wired it into both telemetry paths: spans created through the logger and native OpenTelemetry spans. Before merge, two things need fixing: a nested splunk_ao_context(...) drops the outer simulation_run() scope, and one test edit removed existing coverage. I'd also like to rename the attribute while it's still unreleased.
Please add a PR description that covers:
- the problem this solves
- why the run ID is a span attribute and not baggage or a Resource attribute
- how it's meant to interact with
splunk_ao_context(...),init()and session scopes
Rename the attribute. Please use splunk_ao.simulation.run_id instead of splunk_ao.simulation_run.id. A splunk_ao.simulation.* namespace leaves room for more simulation attributes later, such as scenario or persona IDs, without each one getting its own top-level prefix. Since this hasn't shipped, renaming now costs nothing. Later it would break consumers. Please update the README, ARCHITECTURE.md, CHANGELOG.md, AGENTS.md and the tests to match.
Cross-process propagation. The run ID lives only in a local context variable. It isn't in baggage and isn't carried by the session propagator wrapper. So when a simulation client calls an instrumented agent service over HTTP, the server-side spans won't have the run ID. If grouping across services is in scope, add the run ID to the propagator. If not, please state the limit in the README and ARCHITECTURE.md.
Docs. The new simulation_run_id= parameter on splunk_ao_context(...) is public, but the README and CHANGELOG.md only mention simulation_run(). Please document it, or drop it if simulation_run() is meant to be the only entry point.
| _experiment_id_context.set(None) | ||
| _mode_context.set(_get_mode_or_default(None)) | ||
| set_session_context(None) | ||
| _simulation_run_id_context.set(None) |
There was a problem hiding this comment.
Major. __call__ resets _simulation_run_id_context to None on entry, which breaks the README example. If a conversation wrapped in simulation_run("r") opens splunk_ao_context(session_id=...) or splunk_ao_context(project=...), no span inside that block gets the run ID.
A run ID groups spans; it isn't routing state, so it shouldn't be reset like project, Agent Stream or experiment. Please keep pushing the current value onto _simulation_run_id_stack so __exit__ restores it, but drop this reset and only override the value when simulation_run_id is passed:
| _simulation_run_id_context.set(None) | |
| # Simulation run scope is orthogonal to routing; inherit it unless explicitly overridden. |
Please add a regression test: with simulation_run("r"): with splunk_ao_context(project="p"): ... should export a span that carries "r".
| _span_stack_context.set([]) | ||
| _trace_context.set(None) | ||
| set_session_context(None) | ||
| _simulation_run_id_context.set(None) |
There was a problem hiding this comment.
Major (same root cause). init() also clears an active simulation_run() scope. If someone calls init() inside a run block, the rest of that block loses the run ID. I'd remove this line. reset() (line 1369) is an explicit full reset, so clearing there seems fine.
| assert _simulation_run_id_context.get() is None | ||
|
|
||
|
|
||
| def test_simulation_run_scope_restores_the_previous_value(monkeypatch: pytest.MonkeyPatch) -> None: |
There was a problem hiding this comment.
Major. The new test was inserted partway through test_nested_context_restoration. Everything from # First level context (line 51) onward now runs under test_simulation_run_scope_restores_the_previous_value. That has two effects:
- The original test now only checks that values start as
None, so its nesting coverage is gone. - The nesting block runs without the
AgentStreams,ProjectsandTracespatches or the reset fixture. It only passes becauseget_logger_instanceis monkeypatched.
Please move the new test after the original one and put the nesting block back. With the inheritance fix in decorator.py, the second-level assertion (_simulation_run_id_context.get() is None) should become == "run-1".
|
|
||
| if session_id: | ||
| span.set_attribute("gen_ai.conversation.id", session_id) | ||
| if simulation_run_id: |
There was a problem hiding this comment.
Minor. The two paths treat an empty run ID differently. This check is truthy, so simulation_run("") adds no attribute. SpanConverter.convert_span uses is not None, so the logger path exports "". Since simulation_run() and splunk_ao_context(simulation_run_id=...) are public inputs, I'd validate there and reject empty or non-string IDs with a ValueError. Then both paths can safely use the same check.
| start_time_ns = _to_unix_ns(span.created_at) | ||
| attributes = build_span_attributes(span, session_id) | ||
| if simulation_run_id is not None: | ||
| attributes["splunk_ao.simulation_run.id"] = simulation_run_id |
There was a problem hiding this comment.
Nit. The attribute name is a raw string literal in the converter, otel.py and the tests. Please define one constant next to SPLUNK_AO_SYSTEM in converter/attribute_mapping.py, using the renamed key, so the two paths can't drift:
SPLUNK_AO_SIMULATION_RUN_ID = "splunk_ao.simulation.run_id"
fercor-cisco
left a comment
fercor-cisco
left a comment
There was a problem hiding this comment.
Two suggestions below. Neither blocks the merge; pick them up here or in a follow-up, whichever you prefer.
1. The local and cross-service paths disagree on whitespace in a run ID (src/splunk_ao/simulation_run_context.py:13-17)
validate_simulation_run_id() rejects a blank ID but returns the value unstripped. get_baggage_simulation_run_id() strips it, and OpenTelemetry's baggage extractor trims values anyway. So a run ID read from a file or env var with a trailing newline, such as "run-1\n", is exported as "run-1\n" on the caller's spans and as "run-1" on downstream spans. One run ends up as two groups.
Returning simulation_run_id.strip() keeps both paths consistent. A test case with surrounding whitespace would cover it, and the docstring's "return it unchanged" would need updating too.
2. The new code adds an import cycle (src/splunk_ao/simulation_run_context.py:8)
simulation_run_context → converter.attribute_mapping → converter/__init__.py → span_converter → simulation_run_context. It works today only because every entry point loads splunk_ao.converter first: logger.py imports it before this module, and session_context imports the converter first. If an import is ever reordered so simulation_run_context loads first, import splunk_ao fails with cannot import name 'validate_simulation_run_id'.
Moving SPLUNK_AO_SIMULATION_RUN_ID into simulation_run_context.py (a leaf with no splunk_ao imports) and importing it from there in attribute_mapping, span_converter, otel.py and session_context would remove the cycle.
2 participants
Problem:
A simulation can span multiple conversations, each with its own session ID. The run ID groups those conversations’ telemetry under one simulation without changing their session identity or routing.
Telemetry:
Logger-created and native OTel spans export the run ID as splunk_ao.simulation.run_id. Supported W3C propagation carries it in baggage to downstream services; it is not a Resource attribute because it varies per run, and baggage is the propagation carrier rather than the exported span field.
Context behavior:
Use
splunk_ao_context.simulation_run(...)orsplunk_ao_context(simulation_run_id=...)to set the run ID. Nestedsplunk_ao_context(...)scopes start without the outer run ID unless explicitly given one, then restore the outer ID on exit.init()clears the active run ID. Session and experiment scoping retain their existing behavior.